EDM-3535: Correct isRHEM detection for OCP plugin#694
Conversation
WalkthroughBackend now serves a JSON UI settings endpoint with an ChangesUI Settings Endpoint and Frontend Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 🔧 OpenGrep (1.23.0)apps/ocp-plugin/src/components/AppContext/AppContext.tsx┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.13][ERROR]: unable to find a config; path apps/ocp-plugin/src/utils/apiCalls.ts┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.19][ERROR]: unable to find a config; path proxy/config/config.go┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.31][ERROR]: unable to find a config; path
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@proxy/config/config.go`:
- Around line 27-29: IsRHEMEnabled is an exported function lacking a godoc
comment; add a concise godoc comment immediately above the IsRHEMEnabled
function explaining what it checks (reads the IS_RHEM env var) and what the
boolean return value represents (true when IS_RHEM == "true", false otherwise),
so the exported API is properly documented for users and tools.
In `@proxy/server/settings.go`:
- Line 19: The call to json.NewEncoder(w).Encode(settings) currently ignores its
error; modify the handler that writes settings (the code using
json.NewEncoder(w).Encode(settings), referencing variables w and settings) to
check the returned error, log or record it, and return an appropriate HTTP error
status instead of silently continuing: if Encode fails, write a 500 response
(e.g., http.Error or w.WriteHeader(500) + an error body) and ensure no 200
response is sent; otherwise continue to write the successful JSON as before.
- Around line 14-20: The UISettingsHandler currently serves a top-level public
endpoint and swallows the JSON encode error; add an explicit comment above
UISettingsHandler documenting that this endpoint is intentionally public
(registered on router, not apiRouter), that it only exposes the deployment-wide
flag returned by config.IsRHEMEnabled() and must never include org/user-specific
data, or alternatively move its registration to apiRouter if it should require
middleware.AuthMiddleware/middleware.OrganizationMiddleware; then fix the
ignored encoder error by capturing the return value from
json.NewEncoder(w).Encode(settings) and handling it (log via your existing
logger and respond with an appropriate http error/status if encoding fails).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 62afc2cc-db3c-4815-951b-e2d4e235dd8a
📒 Files selected for processing (5)
apps/ocp-plugin/src/components/AppContext/AppContext.tsxapps/ocp-plugin/src/utils/apiCalls.tsproxy/app.goproxy/config/config.goproxy/server/settings.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@proxy/server/settings.go`:
- Around line 17-23: The handler currently streams JSON directly with
json.NewEncoder(w).Encode(settings), which can commit a 200 before Encode fails;
instead marshal the payload first (e.g., json.Marshal or json.MarshalIndent)
into a byte slice, check the error, and only after successful marshal set the
Content-Type header and write the bytes to w (optionally set Content-Length).
Update the uiSettings write path to replace json.NewEncoder(w).Encode(settings)
with pre-marshalling and return http.Error(..., http.StatusInternalServerError)
only if marshal fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 6476b5d1-52f4-4a89-bb29-abde8634e7fa
📒 Files selected for processing (5)
apps/ocp-plugin/src/components/AppContext/AppContext.tsxproxy/app.goproxy/config/config.goproxy/server/server.goproxy/server/settings.go
8de39cf to
39aaf8e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@proxy/server/settings.go`:
- Around line 26-27: The write error from w.Write(payload) is being ignored;
update the handler containing the w.Write call to log the error before returning
— e.g., capture err from w.Write(payload), call the existing logger (or the
standard log package) like logger.Errorf("write response failed: %v", err) or
log.Printf(...) and then return; reference the w.Write(payload) expression and
its enclosing function to locate where to add the logging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 91948f37-6167-4d43-8057-7885a037508c
📒 Files selected for processing (5)
apps/ocp-plugin/src/components/AppContext/AppContext.tsxproxy/app.goproxy/config/config.goproxy/server/server.goproxy/server/settings.go
jkilzi
left a comment
There was a problem hiding this comment.
Solid fix. The two-part approach (Go bool + runtime /ui-settings fetch) is the right solution for OCP plugin RHEM detection.
Go side: converting IS_RHEM from getEnvVar (string) to parseBoolEnv (bool) eliminates the fragile == "true" check in serveIndexPage. Clean.
Frontend: useUiSettings correctly seeds from window.isRHEM as a fallback and uses AbortController for effect cleanup — no memory leak. Error is swallowed silently which is the right choice for a non-critical settings fetch.
Note — auth: /ui-settings returns isRHEM with no explicit auth check. This is fine since the entire OCP plugin proxy sits behind OpenShift auth anyway.
Nit: the initial useState({ isRHEM: window.isRHEM ?? false }) means that if the RHEM index page already set window.isRHEM = true, there will be a harmless double render (initial true → fetch confirms true). Worth a comment for future readers.
Made-with: Cursor
Made-with: Cursor
39aaf8e to
abe54e1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@proxy/app.go`:
- Around line 75-77: The /ui-settings route gating in app.go is using a raw
string comparison on config.OcpPlugin, so normalize IS_OCP_PLUGIN before this
branch instead of checking only for "true". Update the config handling in
proxy/config/config.go or the route registration logic in app.go so values like
TRUE, 1, or yes are treated as enabled, and keep the change anchored around the
existing OcpPlugin and UISettingsHandler path registration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 8db72aef-e37c-4fd2-9f37-44fc5c5dd94b
📒 Files selected for processing (6)
apps/ocp-plugin/src/components/AppContext/AppContext.tsxapps/ocp-plugin/src/utils/apiCalls.tsproxy/app.goproxy/config/config.goproxy/server/server.goproxy/server/settings.go
| if config.OcpPlugin == "true" { | ||
| apiRouter.HandleFunc("/ui-settings", server.UISettingsHandler).Methods(http.MethodGet) | ||
| } else { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Normalize IS_OCP_PLUGIN before gating this route.
This branch still depends on config.OcpPlugin == "true", but proxy/config/config.go defines OcpPlugin as a raw env string. Values like TRUE, 1, or yes leave /api/ui-settings unregistered, so the OCP plugin falls back to false and the RHEM fix never activates.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@proxy/app.go` around lines 75 - 77, The /ui-settings route gating in app.go
is using a raw string comparison on config.OcpPlugin, so normalize IS_OCP_PLUGIN
before this branch instead of checking only for "true". Update the config
handling in proxy/config/config.go or the route registration logic in app.go so
values like TRUE, 1, or yes are treated as enabled, and keep the change anchored
around the existing OcpPlugin and UISettingsHandler path registration.
The OCP plugin did not receive the "settings" therefore the "isRHEM" detection didn't work there.
Areas affected
settings.isRHEM)Summary
This PR fixes
isRHEMdetection for the OCP plugin by exposing runtime UI settings from the Go proxy and consuming them in the plugin’s app context.apps/ocp-plugin/
apps/ocp-plugin/src/components/AppContext/AppContext.tsx:useUiSettings()hook that initializesisRHEMfromwindow.isRHEM, then fetches${apiProxy}/ui-settings(AbortController-protected).data.isRHEMto a boolean (!!data.isRHEM), writes it back towindow.isRHEM, and updates React state; defaults remain if the fetch fails/aborts.useValuesAppContextnow providessettings: { isRHEM: boolean }instead of a hardcoded emptysettingsobject.apps/ocp-plugin/src/utils/apiCalls.ts:uiProxyis now exported so other modules can reuse the computed UI proxy base URL.proxy/
proxy/config/config.go:IsRHEMnow parsesIS_RHEMas a boolean (parseBoolEnv("IS_RHEM", false)).proxy/server/settings.go:UISettingsHandler, returning JSON{ "isRHEM": <bool> }sourced fromconfig.IsRHEM.proxy/app.go:GET /api/ui-settingsonly whenconfig.OcpPlugin == "true".proxy/server/server.go:serveIndexPagenow checksconfig.IsRHEMas a boolean to choose betweenindexandindex-rhem.Cross-cutting implications
/api/ui-settings) to determineisRHEM, aligning plugin behavior with server/env configuration.libs/ui-components/, it enables consistentsettings.isRHEMavailability to components that already consume it.libs/types/,libs/i18n/,libs/cypress/,apps/standalone/, packaging, container builds, or CI/E2E configuration.